Do not attach ui after parse #200
Conversation
|
Also, @dthree after reading your notice, I think I could offer some help. I'm building a project that makes heavy use of Vorpal :) https://github.com/ccorcos/doug/ |
|
@scotthovestadt mind taking a look at this? |
|
I've been having similar issues with vorpal over the last year. We need to make sure that this patch doesn't break any other functionality, and I know we're light on tests. One of the other issues I've had to work around is the usage of We def need some tests for the functionality this patch brings. I'm not sure how using |
| @@ -193,7 +193,7 @@ var UI = function (_EventEmitter) { | |||
| value: function prompt(options, cb) { | |||
| var _this2 = this; | |||
|
|
|||
| var prompt = undefined; | |||
| var prompt = void 0; | |||
There was a problem hiding this comment.
would prefer the use of the undefined primitive here instead of void 0.
There was a problem hiding this comment.
I think void 0 is an artifact of the build process.
On doing reviews, makes sure you're looking at src and not dist, as the latter is irrelevant.
| @@ -34,7 +34,7 @@ var UI = function (_EventEmitter) { | |||
| function UI() { | |||
| _classCallCheck(this, UI); | |||
|
|
|||
| var _this = _possibleConstructorReturn(this, Object.getPrototypeOf(UI).call(this)); | |||
| var _this = _possibleConstructorReturn(this, (UI.__proto__ || Object.getPrototypeOf(UI)).call(this)); | |||
There was a problem hiding this comment.
__proto__ is deprecated, and use is highly discouraged. With the way the command object is currently passed around, this could be destructive for downstream uses.
Should we should be using UI.prototype.constructor instead.
| @@ -540,7 +540,7 @@ vorpal._onKeypress = function (key, value) { | |||
| vorpal.prompt = function () { | |||
| var _this = this; | |||
|
|
|||
| var options = arguments.length <= 0 || arguments[0] === undefined ? {} : arguments[0]; | |||
| var options = arguments.length > 0 && arguments[0] !== undefined ? arguments[0] : {}; | |||
|
I think this is the most important patch right now. About 4 months ago, some PR broke the |
|
@LongLiveCHIEF I didnt make any changes to |
|
Let me know if there's anything else I can do or when this is published :) |
|
My bad, I'm so used to |
I was also going over some of the details of #169 and can see there has been a lot of discussion around @dthree all said... what do you think is needed in order to accept this PR? |
|
This looks fine as is - it's actually a revert really. |
|
@dthree would love to get this in a release <3 |
|
@dthree can you please publish this? |
There are a few reasons for this PR.
I think its a good idea to be able to use Vorpal simply as a CLI tool without necessarily having to use the interactive shell when using
.parseI was having some issues where I was starting an express server with a Vorpal command and I would listen to SIGINT and SIGTERM to gracefully shutdown the server, but then I would get this bizarre
Error: read EIO closeerror on the next key I typed after exiting withctrl-c. The only way to solve it was withprocess.on('SIGINT', () => process.exit(2)). I don't get this error anymore.Reference Issue: #199
I meant to open up a second PR, but it ended up here as well so why not: #198
It's just adding a simple prototype method on command so you can compose program options. for example.